[UR][L0 v2] Port USM alloc to adapter v2 #18179
Conversation
| @@ -348,19 +348,21 @@ ur_result_t urEnqueueUSMDeviceAllocExp( | |||
| uint32_t numEventsInWaitList, const ur_event_handle_t *phEventWaitList, | |||
There was a problem hiding this comment.
This file is auto-generated. If you want to modify it you need to change scripts/templates/queue_api.cpp.mako (same is true for the corresponding .hpp file)
There was a problem hiding this comment.
But I don't think you need to actually modify it. As far as I understand you wanted to add ur_queue_handle_t param so that you can pass it to EnqueuedPool functions (like getBestFit, etc.). However, if I'm not mistaken, the queue is only used as an identifier by the EnqueuedPool implementation. This means that you can just do something like this in queue_immediate_in_order.cpp:
...
getBestFit(size, alignment, reinterpret_cast<ur_queue_handle_t>(this));
If the EnqueuedPool actually needs to have valid queue handle, then I think it would be better to rewrite EnqueuedPool (which you might need to do anyway, as it has some logic for handling events that is legacy-specific).
|
|
||
| auto device = (type == UR_USM_TYPE_HOST) ? nullptr : hDevice; | ||
|
|
||
| std::vector<ur_event_handle_t> extendedWaitList; |
There was a problem hiding this comment.
You don;t need to create a temporary vector here. commandListManger has getWaitListView function that you should use here (it also accepts an optional extra wait event). You just call: getWaitListView(commandListLocked, phEventWaitList, numEventsInWaitList, originAllocEvent);
| return Ret; | ||
| } | ||
| } else { | ||
| *ppMem = std::get<0>(*asyncAlloc); |
There was a problem hiding this comment.
nit: std::tie(*ppMem, originAllocEvent) = *asyncAlloc;
| const ur_exp_async_usm_alloc_properties_t *pProperties, | ||
| uint32_t numEventsInWaitList, const ur_event_handle_t *phEventWaitList, | ||
| void **ppMem, ur_event_handle_t *phEvent) { | ||
| return enqueueUSMAllocHelper(hQueue, pPool, size, pProperties, |
There was a problem hiding this comment.
Please add TRACK_SCOPE_LATENCY("ur_queue_immediate_in_order_t::enqueueUSMHostAllocExp"); at the beginning of this function (and other function you implement in this patch).
staniewzki
force-pushed
the
async-l0-adapter-v2
branch
2 times, most recently
from
8fc9615 to
4ac080d
Compare
|
|
||
| umf_memory_pool_handle_t hPool = umfPoolByPtr(pMem); | ||
| if (!hPool) { | ||
| return UR_RESULT_SUCCESS; |
There was a problem hiding this comment.
This shouldn't be a success, probably UR_RESULT_ERROR_INVALID_MEM_OBJECT would be appropriate.
There was a problem hiding this comment.
I was looking at umfFree implementation, used by urUSMFree:
umf_result_t umfFree(void *ptr) {
umf_memory_pool_handle_t hPool = umfPoolByPtr(ptr);
if (hPool) {
LOG_DEBUG("calling umfPoolFree(pool=%p, ptr=%p) ...", (void *)hPool,
ptr);
return umfPoolFree(hPool, ptr);
}
return UMF_RESULT_SUCCESS;
}
Here, if hPool is null, success is returned
There was a problem hiding this comment.
In our case, user could provide invalid pointer and we would return success. Considering current umf use-cases in L0 and L0 v2 I don't think it is possible to have a valid pointer that doesn't come from umf pool.
staniewzki
changed the title
|
|
||
| ur_result_t ur_context_handle_t_::retain() { | ||
| RefCount.increment(); | ||
| return UR_RESULT_SUCCESS; | ||
| } | ||
|
|
||
| ur_result_t ur_context_handle_t_::release() { | ||
| asyncPool.cleanupPools(); |
There was a problem hiding this comment.
Shouldn't this be after decrementAndTest?
This PR ports USM alloc enqueue API introduced to L0 adapter in #17112 to L0 adapter v2.